Skip to content

[diffusion] fix: restore nested condition rows in FlowGRPO actor replay - #534

Merged
NancyFyong merged 5 commits into
verl-project:mainfrom
NancyFyong:fix/ref2va-nestedtensor
Sep 8, 2026
Merged

NancyFyong merged 5 commits into
verl-project:mainfrom
NancyFyong:fix/ref2va-nestedtensor

Conversation

@NancyFyong

@NancyFyong NancyFyong commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

What does this PR do?

Fixes a RuntimeError: slice() not supported for NestedTensor on dim=0 that blocks
MiniMax H3 Ref2VA FlowGRPO training on current main. T2VA and FL2VA are
unaffected.

Ref2VA reference condition rows (condition_video_rows / condition_audio_rows)
are padded to a global length and turned into jagged nested tensors by
embeds_padding_2_no_padding. The FlowGRPO diffusion engine never restored them
to dense tensors, so the MiniMax H3 Ref2VA adapter's [:, :count] slice in
diffusers_training_adapter.py crashed before the first training step.

Checklist Before Starting

Test

CPU regression coverage in
tests/pipelines/test_minimax_h3_ref2va_flow_grpo_on_cpu.py:

  • test_ref2va_actor_replays_full_layout_and_scores_only_targets: builds the
    complete Ref2VA replay micro-batch with nested condition rows and drives it
    through PPODiffusersFSDPEngine.prepare_model_inputs, verifying the engine
    restores dense rows before the adapter performs [:, :count] slicing.
  • test_ref2va_engine_unpads_nested_condition_rows: directly covers dense
    restoration and row-count handling for heterogeneous nested rows.
  • test_ref2va_engine_rejects_mismatched_nested_mask: verifies nested rows
    without a nested mask fail closed.

Commands run (project venv, CPU):

PYTHONPATH=$PWD python -m pytest \
  tests/pipelines/test_minimax_h3_ref2va_flow_grpo_on_cpu.py::test_ref2va_actor_replays_full_layout_and_scores_only_targets -q
# 1 passed

PYTHONPATH=$PWD python -m pytest \
  tests/pipelines/test_minimax_h3_flow_grpo_on_cpu.py \
  tests/pipelines/test_minimax_h3_diffusion_nft_on_cpu.py \
  tests/pipelines/test_minimax_h3_ref2va_flow_grpo_on_cpu.py \
  tests/pipelines/test_minimax_h3_ref2va_nft_on_cpu.py \
  tests/workers/test_diffusers_dtype_on_cpu.py -q
# 93 passed

pre-commit run --files \
  verl_omni/workers/engine/fsdp/diffusers_impl.py \
  tests/pipelines/test_minimax_h3_ref2va_flow_grpo_on_cpu.py
# Passed

Negative-control check: temporarily replacing
PPODiffusersFSDPEngine._unpad_condition_rows with a no-op makes the full
actor-replay test fail with the original
RuntimeError: slice(): not supported for NestedTensor on dim=0.

A stacked eight-GPU TinyRandom Ref2VA FlowGRPO smoke also completed one rollout
and actor-update step (training/global_step: 1) and produced eight MP4s plus
one JSONL rollout record.

Design & Code Changes

  • Hoist _unpad_condition_rows from NFTDiffusersFSDPEngine into the shared
    DiffusersFSDPEngine base so it is available to all diffusion engines.
  • Call self._unpad_condition_rows(micro_batch) in
    PPODiffusersFSDPEngine.prepare_model_inputs before delegating to the adapter, so
    condition rows reach the MiniMax H3 Ref2VA adapter as dense padded tensors.
  • Remove the now-duplicated _unpad_condition_rows definition from the NFT engine
    (it inherits the base version; its existing call site still resolves correctly).
  • Leave DPODiffusersFSDPEngine unchanged because there is no Ref2VA DPO pipeline.

Checklist Before Submitting

  • Read the Contribute Guide.
  • Apply pre-commit checks (see Test section).
  • Added engine-entry regression coverage for the restored-rows and
    mismatched-mask paths.

AI assistance

AI assistance (pi coding agent) was used for this change.

A human submitter (NancyFyong) reviewed every changed line and verified the
commands/results above.

Ref2VA reference condition rows are padded to a global length and turned
into jagged nested tensors by embeds_padding_2_no_padding. The FlowGRPO
diffusion engine never restored them, so the MiniMax H3 Ref2VA adapter's
[:, :count] slice on condition_video_rows raised
'RuntimeError: slice() not supported for NestedTensor on dim=0' before the
first training step.

Move _unpad_condition_rows from the DiffusionNFT engine into the shared
DiffusersFSDPEngine base and call it from PPODiffusersFSDPEngine.forward
path so condition rows reach the adapter as dense padded tensors; the
adapter slices the first count rows on dim 1, which is the existing T2VA/
FL2VA behavior.

AI assistance (pi coding agent) was used for this change.

Co-authored-by: pi coding agent
Signed-off-by: NancyFyong <2742092809@qq.com>
Copilot AI lite review requested due to automatic review settings September 4, 2026 08:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The fix is localized to engine-side input preparation, preserves existing NFT behavior via the base method, and is backed by targeted CPU regression tests for both success and validation failure paths.

Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

@NancyFyong

Copy link
Copy Markdown
Collaborator Author

@SamitHuang @zhtmike PTAL

@zhtmike

zhtmike commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

verdict: fix is correct and minimal — hoist is behavior-preserving (body unchanged), all actor-replay/ref/teacher paths go through the one call site, fail-closed on bad masks. one coverage gap, then good to go.

  1. tests pin the helper, not the fix — delete the self._unpad_condition_rows(micro_batch) at verl_omni/workers/engine/fsdp/diffusers_impl.py:983 and the whole suite stays green. better to drive one test through PPODiffusersFSDPEngine.prepare_model_inputs with nested rows; the adapter test at tests/pipelines/test_minimax_h3_ref2va_flow_grpo_on_cpu.py:257 already builds the full ref2va micro_batch, and tests/workers/test_diffusers_fsdp_merged_lora_on_cpu.py:37 shows the object.__new__ + mocked-attrs engine pattern.

  2. PR test list misses tests/pipelines/test_minimax_h3_ref2va_nft_on_cpu.py — the file exercising the method you deleted from the NFT engine. ran it on the PR head (verl-omni-py312, CPU): 20 passed; with your claimed files, 93 total green. add it to the evidence.

  3. DPODiffusersFSDPEngine.prepare_model_inputs (diffusers_impl.py:1119) still doesn't unpad condition rows — fine today since Ref2VA has no DPO pipeline, but that's where this crash reappears if one lands. noting only, nothing to change now.

AI assistance (ZCode) was used for this review.

Drive the Ref2VA actor-replay test through the PPO diffusion engine so removing the condition-row unpadding call reproduces the original NestedTensor slice failure.

AI assistance (pi coding agent) was used for this change.

Co-authored-by: pi coding agent
Signed-off-by: NancyFyong <2742092809@qq.com>
@NancyFyong
NancyFyong force-pushed the fix/ref2va-nestedtensor branch from e18db29 to 3bcfc47 Compare September 7, 2026 10:39
@NancyFyong

Copy link
Copy Markdown
Collaborator Author

Addressed the coverage gap in 3bcfc47.

  • test_ref2va_actor_replays_full_layout_and_scores_only_targets now constructs nested condition rows and calls PPODiffusersFSDPEngine.prepare_model_inputs, so it exercises the PPO call site before continuing through the full Ref2VA adapter replay assertions.
  • Negative control: replacing _unpad_condition_rows with a no-op makes that test fail with the original RuntimeError: slice(): not supported for NestedTensor on dim=0.
  • Added tests/pipelines/test_minimax_h3_ref2va_nft_on_cpu.py to the executed test set; the combined CPU run is 93 passed.
  • Pre-commit passes on both changed files.
  • Left DPO unchanged as suggested because no Ref2VA DPO pipeline exists today.

I also updated the PR test evidence with these results. Thanks for the precise review.

AI assistance (pi coding agent) was used to prepare this response and change.

@zhtmike zhtmike added the ready-for-ci read for running CI label Sep 7, 2026
@github-actions github-actions Bot removed the ready-for-ci read for running CI label Sep 8, 2026
@NancyFyong NancyFyong added the ready-for-ci read for running CI label Sep 8, 2026
@NancyFyong
NancyFyong merged commit 5b9afed into verl-project:main Sep 8, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-ci read for running CI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants